Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The moved table printer now uses snprintf() without an explicit header include (build fragility), and the new AuTest helper hardcodes python3 instead of using the harness interpreter.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes traffic_ctl plugin list so it honors -f json by routing the successful response through the configured printer (matching the behavior of other traffic_ctl commands), and adds AuTest coverage to ensure JSON output is parseable and stable.
Changes:
- Add a
PluginListPrinterand use it fortraffic_ctl plugin listso JSON format works on the success path. - Centralize “JSON-compatible yaml-cpp emitter” configuration via
ts::Yaml::configure_json_emitter()(incl.nullemission). - Add gold tests and test utilities to validate both text output presence and JSON parsing/structure for plugin list output (including empty plugin lists).
File summaries
| File | Description |
|---|---|
| tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py | Adds JSON result-data matcher helper and plugin command wrapper; supports injecting plugin.config lines into the ATS process. |
| tests/gold_tests/traffic_ctl/traffic_ctl_plugin_output.test.py | New AuTest covering plugin list output in JSON and basic text mode smoke validation. |
| tests/gold_tests/traffic_ctl/traffic_ctl_plugin_empty.test.py | New AuTest ensuring empty plugin list emits plugins: [] and JSON parses. |
| src/traffic_ctl/CtrlPrinters.h | Declares new PluginListPrinter. |
| src/traffic_ctl/CtrlPrinters.cc | Implements PluginListPrinter table rendering and switches JSON emission to shared emitter configuration. |
| src/traffic_ctl/CtrlCommands.cc | Selects PluginListPrinter for the plugin list subcommand and routes output through _printer. |
| src/mgmt/rpc/handlers/plugins/Plugins.cc | Ensures empty plugin lists are emitted as an empty sequence (not null). |
| src/mgmt/rpc/handlers/hostdb/HostDB.cc | Ensures empty hostdb partition lists are emitted as an empty sequence (not null). |
| src/config/storage.cc | Routes JSON emission through ts::Yaml::configure_json_emitter(). |
| src/config/ssl_multicert.cc | Routes JSON emission through ts::Yaml::configure_json_emitter(). |
| include/tsutil/YamlCfg.h | Introduces ts::Yaml::configure_json_emitter() to centralize JSON-ish yaml-cpp emitter configuration. |
| include/shared/rpc/yaml_codecs.h | Uses ts::Yaml::configure_json_emitter() for request encoding output. |
| include/mgmt/rpc/jsonrpc/json/YAMLCodec.h | Uses ts::Yaml::configure_json_emitter() and updates related documentation/comments. |
| doc/developer-guide/jsonrpc/jsonrpc-architecture.en.rst | Documents null emission behavior (null vs ~) for JSON compatibility. |
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
fae05ff to
e8dfbee
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The change correctly restores format dispatch for plugin list without altering text output and adds targeted AuTests that validate real JSON parsing and payload structure.
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
[approve ci autest] |
cmcfarlen
left a comment
There was a problem hiding this comment.
LGTM. The printer refactor is behavior-preserving as far as I can tell — dropping the is_error() early-out is fine because BasePrinter::write_output(JSONRPCResponse const&) already handles it identically (error -> prints the error and sets CTRL_EX_ERROR in text mode, fullMsg in JSON mode), so no exit-code or error-path regression. Derived write_output gets the same response.result node the old code decoded, the table code moved verbatim, and -f json does parse at plugin list depth because --format is a root-level option and append_option_data scans and erases option tokens across the whole argv from the root — the same mechanism rpc invoke X -f json already relies on.
One non-blocking note on include/tsutil/YamlCfg.h:54: configure_json_emitter mixes emitter scopes. SetNullFormat(LowerNull) is global, but << YAML::DoubleQuoted << YAML::Flow are local manipulators that only apply to the group opened after they are issued. The comment ("Every emitter whose output reaches a JSON consumer must go through here") reads as order-independent, so a future caller who writes a token first — emitter << YAML::BeginMap; ts::Yaml::configure_json_emitter(emitter); — would get block style with unquoted scalars while nulls still render as null, i.e. silently non-JSON output that looks plausible. Every current caller passes a fresh emitter, so this is latent rather than live. A one-line "call this before emitting anything" in the comment (or an assert) would be enough.
plugin_list() formatted its table straight to std::cout and only consulted the printer on the error path, so --format was silently ignored on success and the output could not be consumed as JSON. Move the table into a PluginListPrinter so the command follows the same path as every other one: the base class dispatches on format, emitting the JSON-RPC envelope for -f json and delegating to the printer for text. Text output is unchanged.
e8dfbee to
dcb7a8e
Compare
Assert the payload through -f json rather than the text table. Column widths are a presentation detail, so pinning them down byte for byte only makes the test brittle against cosmetic changes. Text mode gets a smoke check that it still renders a table. The parse is the point. A gold file would have matched unparseable output indefinitely; the command itself had no autest before this. The empty plugin.config case gets its own test because a populated config never reaches it, and because TrafficCtl hardcodes its ATS process name, so two instances cannot share one file. Adds the plugin_config parameter the DSL was missing. The payload is asserted with validate_json_contains, naming the result node so the check also fails if it ever carries a sibling of data.
This is the first caller to pass a nested expectation, and a nested value compared as a single repr() pair on one line, with the actual in emission order and the expected in source order, meant eyeballing two long inline dicts to find the differing key. Serialise both sides the same way, key-sorted, one per line.
dcb7a8e to
08e0242
Compare
|
Rebased onto master and force-pushed, so here is what changed and why. The rebase. #13609 merged and shared its first three commits with this branch. A squash has no link to its originals, so git replayed already-landed code and reported conflicts in The test helper is gone. The approved version added It was also redundant. All three handlers that write Two smaller fixes in that commit: the separate
One new commit. Unchanged: all three C++ files are byte-identical to the approved revision. Only the autests moved. |
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
Review effort: Lite
Findings: None
traffic_ctl: honor
-f jsonforplugin listTL;DR
traffic_ctl plugin list -f jsonprinted the human-readable table and no JSON.The flag was parsed, a printer was constructed, the server returned a correct
payload — the command just never consulted the printer on the success path.
Given a
plugin.yaml:both of these printed the same thing:
Column 1, char 0 — no JSON at all, not a malformed payload.
The
load_orderandenabled: falsecolumns above only exist because of therecent
plugin.yamlmigration, which makes the payload worth consumingprogrammatically in a way the old single-line
plugin.confignever was.This moves the table into a
PluginListPrinterso the command follows the samepath as every other one, and adds the autests that were impossible to write
before.
Text output is unchanged, byte for byte.
This branch is built on top of
#13609
(traffic_ctl: emit JSON null instead of YAML tilde), so GitHub shows five
commits and fourteen files. Only the top two are mine:
Add autests for traffic_ctl plugin list outputtraffic_ctl: honor -f json for plugin listtraffic_ctl: address review on the JSON emitter helpertraffic_ctl: route JSON emitters through one helpertraffic_ctl: emit JSON null instead of YAML tildeEverything under
src/config/,src/mgmt/,include/, anddoc/developer-guide/jsonrpc/in this diff belongs to #13609 — including theone-line
YAML::NodeType::Sequencechange insrc/mgmt/rpc/handlers/plugins/Plugins.cc. This PR's own changes are limitedto
src/traffic_ctl/andtests/gold_tests/traffic_ctl/. Reviewing thosetwo directories covers it.
Note this is not the
plugin.yamlmigration — that is already on master andis untouched here. #13609 is about the JSON emitter writing
~where JSONrequires
null.Why stacked rather than standalone. On master the server emits
plugins: ~for an empty plugin list, which no JSON parser accepts. So
traffic_ctl_plugin_empty.test.pycannot assert a successful parse until#13609 lands its
LowerNullemitter change together with theYAML::NodeType::Sequencefix inget_plugin_list. The alternative was tocopy that one-line fix into this branch, which would have duplicated an open PR
and set up a merge conflict between the two. Stacking makes the dependency
visible instead of hiding it.
If reviewers would rather see this stand alone, the empty-plugin-list test can
move into #13609 — that PR is what makes the output parseable, so the
assertion arguably belongs there — leaving this PR as the printer fix plus the
populated-config test, both of which pass on master unchanged.
The bug
PluginCommand::plugin_list()touched_printerexactly once, in the errorbranch. Past that it decoded the response and hand-rolled a table into
std::cout:Format-agnostic by construction — every format produced byte-identical output,
whether the source was
plugin.yamlor the legacyplugin.config.-f rpcappeared to work, which made this easy to miss: the wire trace isemitted by the transport layer through
_printer->write_debug(), independentlyof whatever the command itself prints.
Every other command hands the response to the printer and lets
BasePrinter::write_output(JSONRPCResponse const &)branch onis_json_format()— emitting the envelope for JSON, delegating to the derivedwrite_output(YAML::Node const &)for text.plugin listwas the only onebypassing it.
The fix
Three files, and the table code moves verbatim:
CtrlPrinters.h— newPluginListPrinter, alongside the fifteen printersalready there.
CtrlPrinters.cc— the table loop, unchanged, asPluginListPrinter::write_output(YAML::Node const &).<iomanip>moves herewith it.
CtrlCommands.cc— pick the printer per subcommand;plugin_list()reduces to build, invoke, hand off.
JSON then works through the base class. The explicit
response.is_error()early-out disappears with no behavior change: the old code passed the response
to a
GenericPrinter, which resolves to the same non-virtualBasePrinter::write_output(JSONRPCResponse const &)the new code calls.Exit codes are unaffected, including on error. Worth noting for reviewers that
BasePrinter::write_outputsetsApp_Exit_Status_Code = CTRL_EX_ERRORonly intext mode — with
-f jsonit emitsfullMsgand returns before theis_error()block, so an RPC error still exitsCTRL_EX_OK. That ispre-existing and global to
traffic_ctl, unchanged here, and orthogonal tothis PR.
The
--formatflag is documented as a global option with no per-commandcarve-out, so this brings the code in line with documented behavior rather than
adding a new capability.
Why text mode keeps the table
HostDBStatusPrinterandServerStatusPrinterboth just callwrite_output_json(result["data"])in text mode — they have no human formatat all, so bare
traffic_ctl server statusalready prints JSON.plugin listis the only command in this family with a real table, and dropping it to match
would be a user-visible regression for no gain. After this change text is for
humans and
-f jsonis for machines.Tests
Two autests, and the assertion is a real parse, not a gold file. That
distinction is the whole point: a gold file would have matched the table
indefinitely, which is exactly how both the ignored
--formatflag and the~shipped green.traffic_ctl_plugin_output.test.py— populatedplugin.config. Asserts thetext table byte for byte, that
-f jsonparses, and thatresult.data.sourceis correct.traffic_ctl_plugin_empty.test.py— emptyplugin.config. Assertspluginsis[], which is the assertion that needs traffic_ctl: emit JSON null instead of YAML tilde #13609 underneath it.Separate file because a populated config never reaches this case, and
because
TrafficCtlhardcodes its ATS process name, so two instances cannotshare one file.
Both use
plugin.configrather thanplugin.yaml: the autest ATS extensionregisters
plugin.configas a Disk file but has noplugin.yamlequivalent,so the DSL cannot write one. This leaves the
load_ordercolumn and thedisabledstatus uncovered — both are reachable only throughplugin.yaml.Adding that registration is a reasonable follow-up; it is not needed to prove
the format dispatch works, which is what this PR changes.
The DSL had no
plugin()builder, so this adds one, plus aplugin_configparameter mirroring the existing
records_yaml, and two assertion helpers:validate_json_parses()— pipes throughjson.loadand asserts exit 0.validate_json_data_contains()— same comparison as the existingvalidate_json_contains, but descends intoresult.datafirst. The existinghelper only reaches top-level keys, and with
-f jsonthose are justjsonrpc,resultandid, so payload fields were unreachable.The new helper keeps its inline script single quoted and passes expected values
as one
shlex.quote'd JSON argument. The existingvalidate_json_containsinterpolates them straight into a double-quoted shell word, so a value
containing an apostrophe raises
SyntaxError, one containing$is silentlyshell-expanded before the comparison, and
$(...)executes. Not fixed here tokeep the diff scoped, but it is a live footgun in that helper.
Verified the tests are a real regression guard rather than passing for the
wrong reason: feeding the captured pre-fix output to
json.loadreproducesExpecting value: line 1 column 1 (char 0)and exits 1.Out of scope
-f yamlis not a formattraffic_ctlsupports._Fmt_str_to_enumholdsonly
jsonandrpc,FormatFlagshas no YAML member, and--formatdocuments
{json|rpc}.parse_print_optslooks the string up and silentlykeeps
NOT_SETon a miss, so-f yaml— and any other unknown value — isignored on every command, not just this one. Rejecting unknown format values
is a separate change with wider blast radius.
Scalars are emitted as strings. The JSON emitter double-quotes every
scalar and
YAML::Nodehas already lost the type, soenabledarrives as"true"andindexas"1". Pre-existing and global totraffic_ctl; theexisting
validate_json_contains(initialized_done='true')assertion dependson it.
Known coverage gaps. The
load_ordercolumn and thedisabledstatus areuntested:
plugin.confighardcodesload_order = -1for every entry, so thewide header and the
--fallback are unreachable, and the autest ATS extensionregisters no
plugin.yamlDisk file for the DSL to write. The error path isalso untested — the
is_error()early-out this PR removes has no autestexercising it. Both are worth follow-ups.
admin_plugin_get_listis undocumented. It appears nowhere injsonrpc-api.en.rst, while its siblingadmin_plugin_send_basic_msgisreferenced from the
plugin msgentry. Left alone here; a doc-only follow-up.Why it matters now
#13609 fixes
traffic_ctlemitting YAML's~where JSON requiresnull, andtwo commands hit it:
hostdb statuson an empty HostDB, andplugin listwithno plugins loaded. Its
plugin listhalf could not be asserted, because-f jsonproduced no JSON to parse — that is the gap this PR closes, which iswhy it sits on top rather than beside.
The
plugin.yamlmigration is the other reason this matters now.plugin listexists to introspect a format that carries per-entry state —
load_order,enabled— and the whole point of a-f jsonon that command is lettingtooling read it. A flag that silently returns a fixed-width table instead
defeats that.